Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verifiable Consensus Data #40

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

GalRogozinski
Copy link
Contributor

@GalRogozinski GalRogozinski commented Jan 30, 2024

Verify that attestation/sync-committee consensus data will be included by beacon chain

Copy link
Contributor

@MatheusFranco99 MatheusFranco99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!!

Comment on lines 109 to 112
// addition
if bytes.Equal(attestationDataByts, data) {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nil or empty check right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the attestation data we got from our own node is the same as the leader's (it should happen if the nodes are synced to the same head) then no need to perform any more validation and we stop

This assumes the operator trusts his beacon node (most of the time they run their own I think).

If the data is nil it should fail I think when we do deocode or validate

Comment on lines 91 to 96
}
blockEpoch := getBeaconNode().getEpochBySlot(proof.Message.slot)
domain := GetBeaconNode().DomainData(blockEpoch, spec.DomainProposer)
root := spec.ComputeEthSigningRoot(beaconRoot, domain)
validator := GetBeaconNode.GetValidator("HEAD", proof.Message.ProposerIndex)
return bls.Verify(sig, validator.PubKey, root)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this check, we're sure that the block proposed as head exists. But we, still, can't be sure if a malicious operator is sending us an old block to attest, correct?
Perhaps, if we had some state that persisted through all duties, we could do a lower limit check for the slot/epoch. This is some future discussion but what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can compare against the block we got from our node simply without persisting state.
But I see it as a future improvement outside this SIP

@GalRogozinski
Copy link
Contributor Author

I need to add a check that the beaconHeaderProof is assigned by a validator that was assigned the duty

@alonmuroch
Copy link
Contributor

Why can't a local node request the block from the local beacon node and see it exists?

Copy link
Contributor Author

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't a local node request the block from the local beacon node and see it exists?

it can but the block may be missing there. We first check the local node and then the proof

Comment on lines +166 to +179
func isSourceJustified(attestationData)) error {
currentEpoch := network.EstimatedCurrentEpoch()
// https://ethereum.github.io/beacon-APIs/#/Beacon/getStateFinalityCheckpoints
checkpoints := getBeaconNode().getFinalityCheckpoints()
if attestationData.Target.Epoch == currentEpoch {
if attestationData.Source.Root != checkpoints.data.currentJustifiedRoot {
return Errors.New("source is not expected currently justified root")
}
} else if attestationData.Source.Root!= checkpoints.data.previousJustifiedRoot {
return Errors.New("source is not expected previously justified root")
}

return nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checks exists in phase0.

In deneb it is hidden inside the participation flags that check eligibility for rewards. There is still an assert statement for this check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants